Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: refactor login processes #162

Merged
merged 3 commits into from
Aug 19, 2024
Merged

Conversation

aversini
Copy link
Collaborator

@aversini aversini commented Aug 19, 2024

User description

BREAKING CHANGE: this is dependent on auth-server v8+


PR Type

Enhancement, Bug fix


Description

  • Refactored the login process by introducing new API types LOGIN and REFRESH.
  • Removed the fingerprint logic and related parameters across multiple files.
  • Simplified types and reducer logic by removing authenticationType.
  • Updated the login function to use loginUser instead of authenticateUser.
  • Removed the @versini/ui-fingerprint dependency from package.json.

Changes walkthrough 📝

Relevant files
Enhancement
constants.ts
Add new API types for login and refresh                                   

packages/auth-common/src/components/constants.ts

  • Added new API types LOGIN and REFRESH.
+2/-1     
constants.ts
Update GraphQL query parameters for authentication             

packages/auth-provider/src/common/constants.ts

  • Replaced fingerprint with sessionExpiration in VERIFY_AUTHENTICATION.
  • +2/-2     
    types.d.ts
    Simplify and update authentication-related types                 

    packages/auth-provider/src/common/types.d.ts

  • Removed AuthenticationTypes type.
  • Updated RestCallProps and LoginProps types.
  • +2/-17   
    utilities.ts
    Refactor authentication utilities and remove fingerprint 

    packages/auth-provider/src/common/utilities.ts

  • Removed fingerprint parameter from functions.
  • Renamed authenticateUser to loginUser.
  • Changed API type from AUTHENTICATE to LOGIN and REFRESH.
  • +3/-29   
    AuthContext.ts
    Simplify AuthContext by removing authenticationType           

    packages/auth-provider/src/components/AuthProvider/AuthContext.ts

    • Removed authenticationType from AuthContext.
    +0/-1     
    AuthProvider.tsx
    Refactor AuthProvider to remove fingerprint and update login

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx

  • Removed fingerprint logic and related effects.
  • Updated login function to use loginUser.
  • +40/-103
    reducer.ts
    Simplify reducer by removing authenticationType                   

    packages/auth-provider/src/components/AuthProvider/reducer.ts

    • Removed authenticationType from reducer actions.
    +0/-2     
    Dependencies
    package.json
    Remove unused fingerprint dependency                                         

    packages/auth-provider/package.json

    • Removed @versini/ui-fingerprint dependency.
    +1/-4     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    BREAKING CHANGE: this is dependent on auth-server v8+
    @qodo-merge-pro qodo-merge-pro bot added enhancement New feature or request Bug fix labels Aug 19, 2024
    Copy link

    qodo-merge-pro bot commented Aug 19, 2024

    CI Failure Feedback 🧐

    (Checks updated until commit 0fa67e5)

    Action: inspect (20.x)

    Failed stage: Run pnpm install --ignore-scripts [❌]

    Failure summary:

    The action failed due to an outdated lockfile issue with pnpm:

  • The pnpm-lock.yaml file is not up to date with the package.json file for the auth-provider package.
  • This discrepancy causes the frozen-lockfile installation to fail, which is the default setting in CI
    environments.
  • The lockfile contains specifiers that do not match those in package.json, leading to the error.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    147:  COREPACK_ENABLE_STRICT: 0
    148:  PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
    149:  ##[endgroup]
    150:  Scope: all 5 workspace projects
    151:  ERR_PNPM_OUTDATED_LOCKFILE  Cannot install with "frozen-lockfile" because pnpm-lock.yaml is not up to date with packages/auth-provider/package.json
    152:  Note that in CI environments this setting is true by default. If you still need to run install in such cases, use "pnpm install --no-frozen-lockfile"
    153:  Failure reason:
    154:  specifiers in the lockfile ({"@simplewebauthn/browser":"10.0.0","@versini/auth-common":"workspace:../auth-common","@versini/ui-fingerprint":"1.0.1","@versini/ui-hooks":"4.0.1","jose":"5.6.3","uuid":"10.0.0","react":"18.3.1","react-dom":"18.3.1"}) don't match specs in package.json ({"react":"18.3.1","react-dom":"18.3.1","@simplewebauthn/browser":"10.0.0","@versini/auth-common":"workspace:../auth-common","@versini/ui-hooks":"4.0.1","jose":"5.6.3","uuid":"10.0.0"})
    155:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Function Renaming
    The authenticateUser function has been renamed to loginUser. This change might affect other parts of the codebase that are not visible in this PR.

    Login Logic Modification
    The login function has been significantly refactored. The type parameter has been removed, and the function now always uses AUTH_TYPES.CODE. This change might affect the flexibility of the login process.

    Type Definitions Update
    Several type definitions have been removed or modified, including AuthenticationTypes and changes to AuthState. This might cause type errors in other parts of the application.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a timeout mechanism for the login process

    Consider adding a timeout mechanism for the login process to prevent indefinite
    waiting in case of network issues or server unresponsiveness.

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx [209-225]

     const login: LoginProps = async (username, password): Promise<boolean> => {
       dispatch({ type: ACTION_TYPE_LOADING, payload: { isLoading: true } });
       removeLocalStorage();
     
       const _nonce = uuidv4();
       setNonce(_nonce);
     
       logger("login: Logging in with password");
     
       const type = AUTH_TYPES.CODE;
       const { code_verifier, code_challenge } = await pkceChallengePair();
    -  const preResponse = await getPreAuthCode({
    -    nonce: _nonce,
    -    clientId,
    -    code_challenge,
    -  });
     
    +  const timeoutPromise = new Promise((_, reject) =>
    +    setTimeout(() => reject(new Error("Login timeout")), 30000)
    +  );
    +
    +  try {
    +    const preResponse = await Promise.race([
    +      getPreAuthCode({
    +        nonce: _nonce,
    +        clientId,
    +        code_challenge,
    +      }),
    +      timeoutPromise
    +    ]);
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Implementing a timeout mechanism for the login process is an important enhancement that prevents indefinite waiting due to network issues or server unresponsiveness, significantly improving the reliability of the application.

    9
    Rename the function to better reflect its purpose

    Consider using a more descriptive name for the loginUser function, such as
    authenticateUser, to better reflect its purpose of handling both login and
    authentication processes.

    packages/auth-provider/src/common/utilities.ts [108]

    -export const loginUser = async ({
    +export const authenticateUser = async ({
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to rename loginUser to authenticateUser is reasonable as it may better reflect the function's purpose. However, the current name is not incorrect, and the change is more about clarity than necessity.

    5
    Error handling
    Add error handling and logging for unsuccessful login attempts

    Consider adding error handling and logging for the case when the login response
    status is not successful. This will provide better debugging information and user
    feedback.

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx [238-254]

     if (response.status) {
       setIdToken(response.idToken);
       setAccessToken(response.accessToken);
       setRefreshToken(response.refreshToken);
       dispatch({
         type: ACTION_TYPE_LOGIN,
         payload: {
           user: {
             userId: response.userId as string,
             username,
           },
         },
       });
       return true;
    +} else {
    +  logger("login: Login failed", response.error || "Unknown error");
    +  removeStateAndLocalStorage(LOGIN_ERROR);
    +  return false;
     }
    -removeStateAndLocalStorage(LOGIN_ERROR);
    -return false;
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling and logging for unsuccessful login attempts is a valuable enhancement that improves debugging and user feedback, making it a significant improvement to the code.

    8
    Possible issue
    Add a check for the existence of the user ID in the login response

    Consider adding a check for the existence of the response.userId before using it in
    the dispatch payload. This will prevent potential runtime errors if the userId is
    undefined.

    packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx [242-250]

    -dispatch({
    -  type: ACTION_TYPE_LOGIN,
    -  payload: {
    -    user: {
    -      userId: response.userId as string,
    -      username,
    +if (response.userId) {
    +  dispatch({
    +    type: ACTION_TYPE_LOGIN,
    +    payload: {
    +      user: {
    +        userId: response.userId,
    +        username,
    +      },
         },
    -  },
    -});
    +  });
    +} else {
    +  logger("login: User ID is missing from the response");
    +  removeStateAndLocalStorage(LOGIN_ERROR);
    +  return false;
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Checking for the existence of response.userId before using it helps prevent potential runtime errors, which is a good practice for ensuring code robustness.

    7

    Copy link

    Bundle Size

    Status File Size (Gzip) Limits
    index.js 14.26 KB (-3.54 KB -19.89%) 15 kb

    Overall bundle size: 14.26 KB (-3.54 KB -19.89%)
    Overall status: ✅

    @aversini aversini merged commit 5902888 into main Aug 19, 2024
    4 checks passed
    @aversini aversini deleted the feat!-refactor-login-processes branch August 19, 2024 22:06
    @aversini aversini mentioned this pull request Aug 19, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant